Patch security hardening improvements 485 #486
Conversation
Changes in file .github/actions/check-control/action.yml: * security hardening of user-controlled inputs Changes in file .github/actions/run-minimal-acceptance-tests/action.yml: * security hardening of user-controlled inputs Changes in file .github/actions/setup-py-reqs/action.yml: * security hardening of user-controlled inputs Changes in file .github/actions/test-reporter-upload/action.yml: * security hardening of user-controlled inputs
Changes in file .github/actions/check-control/action.yml: * fix CWE-20 from double escapping quotes Changes in file .github/actions/run-minimal-acceptance-tests/action.yml: * fix CWE-20 from double escapping quotes Changes in file .github/actions/setup-py-reqs/action.yml: * fix CWE-20 from double escapping quotes
Changes in file .github/actions/check-control/action.yml: * refactored how sha is normalized and sanitized Changes in file .github/actions/run-minimal-acceptance-tests/action.yml: * refactored how sha is normalized and sanitized Changes in file .github/actions/setup-py-reqs/action.yml: * refactored how sha is normalized and sanitized
Changes in file .github/actions/check-control/action.yml: * minor refactor Changes in file .github/actions/run-minimal-acceptance-tests/action.yml: * minor refactor Changes in file .github/actions/setup-py-reqs/action.yml: * minor refactor
Changes in file .github/actions/check-control/action.yml: * removed faulty conditional Changes in file .github/actions/run-minimal-acceptance-tests/action.yml: * removed faulty conditional Changes in file .github/actions/setup-py-reqs/action.yml: * removed faulty conditional
reactive-firewall
added
enhancement
WalkthroughAdds strict input normalization and validation for target SHAs, propagates sanitized SHAs and identifiers via environment variables, hardens GitHub Actions API call inputs (name/title), introduces repository-scoped guards and github-token scaffolding for certain steps, and makes several workflow checks fail early by exiting with non-zero status. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as CI_INPUT_TARGET_SHA
participant Normalizer as Normalizer (trim/strip/validate)
participant Resolver as Ref Resolver
participant Validator as Hex Validator
participant Output as GITHUB_OUTPUT/GITHUB_ENV
Input->>Normalizer: raw input
Normalizer->>Normalizer: trim, strip quotes, remove non-printables, reject leading '-'
Normalizer-->>Validator: sanitized token
alt token is 40-char hex
Validator->>Output: export as resolved SHA
else resolve as ref
Validator->>Resolver: try as full ref
Resolver->>Resolver: try refs/heads/<name>
Resolver->>Resolver: try refs/tags/<name>
Resolver-->>Validator: resolved commit SHA or error
Validator->>Output: verify 40-char hex -> export
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Here's the code health analysis summary for commits Analysis Summary
Code Coverage Report
|
Changes in file .github/actions/check-control/action.yml: * Improved hardening Changes in file .github/actions/checkout-and-rebuild/action.yml: * Improved hardening Changes in file .github/actions/run-minimal-acceptance-tests/action.yml: * Improved hardening Changes in file .github/actions/setup-py-reqs/action.yml: * Improved hardening Changes in file .github/workflows/CI-CHGLOG.yml: * Improved hardening Changes in file .github/workflows/CI-DOCS.yml: * Improved hardening Changes in file .github/workflows/CI-MATs.yml: * Improved hardening Changes in file .github/workflows/Tests.yml: * Improved hardening
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
.github/actions/check-control/action.yml(5 hunks).github/actions/checkout-and-rebuild/action.yml(1 hunks).github/actions/run-minimal-acceptance-tests/action.yml(1 hunks).github/actions/setup-py-reqs/action.yml(1 hunks).github/actions/test-reporter-upload/action.yml(1 hunks).github/workflows/CI-CHGLOG.yml(1 hunks).github/workflows/CI-DOCS.yml(1 hunks).github/workflows/CI-MATs.yml(3 hunks).github/workflows/Tests.yml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.github/**
⚙️ CodeRabbit configuration file
.github/**: * When the project is hosted on GitHub: All GitHub-specific configurations, templates,
and tools should be found in the '.github' directory tree.
- 'actionlint' erroneously generates false positives when dealing with
GitHub's${{ ... }}syntax in conditionals.- 'actionlint' erroneously generates incorrect solutions when suggesting the removal of
valid${{ ... }}syntax.
Files:
.github/actions/checkout-and-rebuild/action.yml.github/workflows/CI-CHGLOG.yml.github/actions/setup-py-reqs/action.yml.github/actions/check-control/action.yml.github/workflows/CI-DOCS.yml.github/actions/test-reporter-upload/action.yml.github/actions/run-minimal-acceptance-tests/action.yml.github/workflows/CI-MATs.yml.github/workflows/Tests.yml
🔇 Additional comments (13)
.github/workflows/CI-DOCS.yml (1)
55-62: Security hardening: early failure on upstream workflow failure.The addition of
exit 1on Line 61 implements an immediate failure path when the upstream CI-MATs workflow does not conclude with "success". This creates a defense-in-depth pattern:
- Early exit: The check step fails immediately if CI-MATs failed
- Step-level gating: Subsequent steps use
success()conditionals (e.g., Line 64) to skip on failure- Job-level gating: Downstream jobs (e.g., DOCS on Line 124) skip when
should_runis falseThis redundant protection layer hardens the workflow against accidental execution of dependent jobs when prerequisites fail, aligning with the PR's stated security hardening objectives.
.github/workflows/CI-MATs.yml (3)
62-69: Early failure path correctly implemented.The
exit 1statement when CI-BUILD fails enforces fail-fast behavior, preventing downstream jobs from using stale or invalid build artifacts when the build workflow did not succeed.
71-71: Repository-scoped guards appropriately harden sensitive operations.Repository checks (
github.repository == 'reactive-firewall-org/multicast') are consistently applied across job entry points and artifact operations. These guards prevent unintended execution in forks, mirrors, or other contexts and align with defense-in-depth principles. Token scaffolding via${{ env.GH_TOKEN }}is properly implemented and used consistently throughout.Also applies to: 80-80, 110-111, 124-124, 127-127, 135-135
104-111: Verify hardcoded repository scope does not break legitimate use cases.Line 110 hardcodes
repository: reactive-firewall-org/multicastin the checkout action, ensuring the workflow always checks out the canonical repository regardless of where the workflow runs. This is intentional for supply-chain security but may prevent testing this workflow in forks or private mirrors.Confirm this is the intended behavior and that there is no requirement to support fork testing or mirror repositories with this workflow.
.github/workflows/CI-CHGLOG.yml (1)
58-65: Early failure enforcement aligns with security hardening objectives.The addition of
exit 1in the else branch enforces immediate failure when the upstream workflow conclusion is not "success". This creates a defense-in-depth control flow:
- The check step exits non-zero, causing the job to fail
- Downstream steps remain gated by
success()conditions (lines 67, 93, 118, 121, 129) and explicitshould_run == 'true'checks- Dependent jobs (line 148) can still inspect the
should_runoutput since it's set before the exitThe logic is sound and integrates correctly with existing guards across the workflow.
.github/actions/checkout-and-rebuild/action.yml (1)
75-79: Env var usage for SHA is correct and aligns with security hardening.The redirection of the SHA through CI_INPUT_TARGET_SHA environment variable supports upstream validation in caller actions and prevents direct expansion of potentially untrusted input in the shell step. This is consistent with the broader PR's validation approach.
.github/actions/test-reporter-upload/action.yml (1)
348-349: Environment variable parameterization for Coveralls integration is correct.The use of intermediate environment variables for service job identifiers follows GitHub Actions best practices and enables consistent propagation of build metadata downstream.
Also applies to: 352-354
.github/actions/setup-py-reqs/action.yml (1)
68-136: Comprehensive SHA validation and normalization logic is well-designed.The multi-stage validation approach effectively mitigates injection attacks and ensures deterministic SHA resolution. The order of reference resolution (full refs → refs/heads/ → refs/tags/) and the mandatory 40-character hexadecimal format requirement provide strong guarantees. Error messages are specific and helpful for debugging.
.github/workflows/Tests.yml (2)
85-85: Early failure path correctly enforces fail-fast semantics.When the upstream MATs workflow does not succeed, immediately exiting with code 1 prevents dependent jobs from attempting to run and consumes fewer resources. This is the correct behavior for a gating condition.
127-128: Repository-scoped checkout with explicit token improves security posture.Specifying
repository: reactive-firewall-org/multicastandgithub-tokenexplicitly guards against accidental checkout of an unintended repository and ensures proper authentication scoping..github/actions/run-minimal-acceptance-tests/action.yml (1)
83-151: SHA validation logic is consistent with setup-py-reqs and implements comprehensive input sanitization.The parallel implementation of validation across actions is intentional for modularity. All stages of validation—control character rejection, normalization, dash-prefix prevention, and multi-path reference resolution—are correctly implemented.
.github/actions/check-control/action.yml (2)
110-177: SHA validation logic is comprehensive and consistent with other actions in the PR.The multi-stage validation—control character rejection, whitespace normalization, quote stripping, dash-prefix prevention, and reference resolution—is correctly implemented and produces a guaranteed 40-character hexadecimal SHA.
221-350: Name and title sanitization pattern is well-designed but has variable naming consistency issues (see separate comment for critical bugs).The approach of building a sanitized field with the
output[title]=prefix ensures proper formatting for the GitHub Check Runs API. However, the inconsistency between definedname_inputvariable and the referenced$input_nameundermines the implementation.Once the undefined variable references are corrected, verify that the check-control action can successfully create, update, and complete GitHub checks with the provided name and title values by running a test workflow.
github-project-automation
bot
moved this from In progress
to Regressions
in Multicast Project
Changes in file .github/actions/check-control/action.yml: * use variable for name and title inputs * fix check for details URL to compar with DEFAULT correctly
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/actions/check-control/action.yml (3)
245-275: 🔴 Critical: Undefined variable at line 268 will cause API call to fail.Line 268 references
$input_name, which is never defined in this step. The variable is assigned as$name_inputon line 255. The GitHub CLI command will pass an empty or unset value for thenameparameter, causing the check creation to fail or succeed with incorrect data.Apply this fix:
CHECK_ID=$(gh api --method POST -H "Accept: application/vnd.github+json" \ /repos/reactive-firewall-org/multicast/check-runs \ - -f "name=$input_name" -f "head_sha=${{ steps.output_sha.outputs.sha }}" \ + -f "name=$name_input" -f "head_sha=${{ steps.output_sha.outputs.sha }}" \
300-328: 🔴 Critical: Undefined variable at line 323 will cause API call to fail.Line 323 references
$input_name, which is never defined in this step. The variable is assigned as$name_inputon line 310. The GitHub CLI command will pass an empty or unset value for thenameparameter, causing the check update to fail or execute with incorrect data.Apply this fix:
gh api --method PATCH -H "Accept: application/vnd.github+json" \ /repos/reactive-firewall-org/multicast/check-runs/${{ steps.output_check_id.outputs.check_id }} \ - -f "name=$input_name" -f "head_sha=${{ steps.output_sha.outputs.sha }}" \ + -f "name=$name_input" -f "head_sha=${{ steps.output_sha.outputs.sha }}" \
329-358: 🔴 Critical: Undefined variable at line 352 will cause API call to fail.Line 352 references
$input_name, which is never defined in this step. The variable is assigned as$name_inputon line 339. The GitHub CLI command will pass an empty or unset value for thenameparameter, causing the check completion to fail or execute with incorrect data.Apply this fix:
gh api --method PATCH -H "Accept: application/vnd.github+json" \ /repos/reactive-firewall-org/multicast/check-runs/${{ steps.output_check_id.outputs.check_id }} \ - -f "name=$input_name" -f "head_sha=${{ steps.output_sha.outputs.sha }}" \ + -f "name=$name_input" -f "head_sha=${{ steps.output_sha.outputs.sha }}" \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
.github/actions/check-control/action.yml(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.github/**
⚙️ CodeRabbit configuration file
.github/**: * When the project is hosted on GitHub: All GitHub-specific configurations, templates,
and tools should be found in the '.github' directory tree.
- 'actionlint' erroneously generates false positives when dealing with
GitHub's${{ ... }}syntax in conditionals.- 'actionlint' erroneously generates incorrect solutions when suggesting the removal of
valid${{ ... }}syntax.
Files:
.github/actions/check-control/action.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check:
- GitHub Check:
- GitHub Check: makefile-lint
🔇 Additional comments (2)
.github/actions/check-control/action.yml (2)
109-177: ✅ Comprehensive SHA input validation and normalization.The
output_shastep correctly implements defense-in-depth:
- Rejects control characters and inputs starting with dashes.
- Normalizes quotes and whitespace.
- Validates direct 40-character hex SHAs.
- Falls back to ref resolution with explicit namespace precedence (refs/heads/, refs/tags/) and clear error messages on failure.
- Final validation ensures the resolved value is always a 40-char commit SHA.
The repository scope guard on line 110 is appropriate.
214-244: ✅ Create New Check: variable usage is correct.Line 237 correctly references
$name_input, which is assigned on line 224 from theCHECK_NAME_INPUTenvironment variable. No issues here.
This work contributes to GHI #488 Changes in file .github/actions/check-control/action.yml: * fix regression in variable name
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
|
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
reactive-firewall
temporarily deployed
to
Testing
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
.github/actions/check-control/action.yml(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.github/**
⚙️ CodeRabbit configuration file
.github/**: * When the project is hosted on GitHub: All GitHub-specific configurations, templates,
and tools should be found in the '.github' directory tree.
- 'actionlint' erroneously generates false positives when dealing with
GitHub's${{ ... }}syntax in conditionals.- 'actionlint' erroneously generates incorrect solutions when suggesting the removal of
valid${{ ... }}syntax.
Files:
.github/actions/check-control/action.yml
🔇 Additional comments (1)
.github/actions/check-control/action.yml (1)
109-178: ✅ Comprehensive SHA validation and normalization pipeline is well-structured.The
output_shastep implements defense-in-depth hardening: control-character rejection, quote/whitespace normalization, dash-prefix check, and multi-level git ref resolution (full path → refs/heads/ → refs/tags/). Early exits and explicit error messages are appropriate. Repository-scoped guard on line 110 is an intentional hardening choice.
github-project-automation
bot
moved this from Regressions
to Resolved
in Multicast Project
Patch Notes
Impacted GHI
Summary by CodeRabbit
Chores
Bug Fixes